Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

✨ Make directory-not-empty error more self-explanatory #1943

Merged
merged 1 commit into from
Jan 15, 2021
Merged

✨ Make directory-not-empty error more self-explanatory #1943

merged 1 commit into from
Jan 15, 2021

Conversation

justinsb
Copy link
Contributor

@justinsb justinsb commented Jan 13, 2021 •
edited
Loading

When I encountered the error, I had to go to the source to understand
the problem. Made the message more user-focused.

Also skipped printing the allowlist, both to fit into lint line-length
limits and to avoid overwhelming the user.

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 13, 2021
@justinsb justinsb force-pushed the directory_not_empty_message branch from 361a400 to ada8cd3 Compare January 13, 2021 12:50
@justinsb justinsb changed the title Make directory-not-empty error more self-explanatory ✨ Make directory-not-empty error more self-explanatory Jan 13, 2021
@justinsb justinsb force-pushed the directory_not_empty_message branch from ada8cd3 to b71349c Compare January 13, 2021 13:18
@justinsb justinsb mentioned this pull request Jan 13, 2021
Merged
Adirio
Adirio reviewed Jan 13, 2021
View reviewed changes
pkg/plugins/golang/v3/init.go Outdated
@@ -188,7 +187,7 @@ func checkDir() error {
return err
}
if info.Name() != "go.mod" && !strings.HasPrefix(info.Name(), ".") {
return errors.New("only the go.mod and files with the prefix \"(.)\" are allowed before the init")
return fmt.Errorf("target directory is not empty: found existing file %q", path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: I think the file name should be enough

Suggested change
return fmt.Errorf("target directory is not empty: found existing file %q", path)
return fmt.Errorf("target directory is not empty: found existing file %q", info.Name())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started with that actually and switched to path :-) The root cause here was that we were finding files like .git/HEAD. info.Name() prints "HEAD", but path prints ".git/HEAD".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we not still clarifying what files are allowed? See that any file with the prefix (.) is allowed. E.g. .gitignore

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought was that maybe the user doesn't care, but it is a good idea to try to reference the idea that some files are allowed, so I added it back! Thanks!

@justinsb
Copy link
Contributor Author

/retest pull-kubebuilder-e2e-k8s-1-16-2

I can't see how that failure could be related, so rolling the dice again!

@k8s-ci-robot
Copy link
Contributor

@justinsb: The /retest command does not accept any targets.
The following commands are available to trigger jobs:

  • /test pull-kubebuilder-test
  • /test pull-kubebuilder-e2e-k8s-1-18-0
  • /test pull-kubebuilder-e2e-k8s-1-17-0
  • /test pull-kubebuilder-e2e-k8s-1-16-2
  • /test pull-kubebuilder-e2e-k8s-1-15-3
  • /test pull-kubebuilder-e2e-k8s-1-14-1

Use /test all to run all jobs.

In response to this:

/retest pull-kubebuilder-e2e-k8s-1-16-2

I can't see how that failure could be related, so rolling the dice again!

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@justinsb
Copy link
Contributor Author

/test pull-kubebuilder-e2e-k8s-1-16-2

estroz
estroz approved these changes Jan 13, 2021
View reviewed changes
Copy link
Contributor

@estroz estroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 13, 2021
@Adirio
Copy link
Contributor

Adirio commented Jan 13, 2021

My only question is the one raised in #1944 (comment)
I will lgtm and hold it, If you consider that we should do it in two PRs just remove the hold and it will merge. If you opt into a single PR then close it and update the other PR accordingly.

/lgtm
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 13, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 13, 2021
@justinsb
Make directory-not-empty error more self-explanatory
20df76b 
When I encountered the error, I had to go to the source to understand
the problem.  Made the message more user-focused.

Also skipped printing the allowlist, both to fit into lint line-length
limits and to avoid overwhelming the user.
@justinsb justinsb force-pushed the directory_not_empty_message branch from b71349c to 20df76b Compare January 14, 2021 16:08
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 14, 2021
estroz
estroz approved these changes Jan 14, 2021
View reviewed changes
Copy link
Contributor

@estroz estroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 14, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: estroz, justinsb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@justinsb
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 15, 2021
@k8s-ci-robot k8s-ci-robot merged commit 1001d36 into kubernetes-sigs:master Jan 15, 2021
@camilamacedo86 camilamacedo86 mentioned this pull request Feb 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Adirio Adirio Adirio left review comments

@estroz estroz estroz approved these changes

@camilamacedo86 camilamacedo86 Awaiting requested review from camilamacedo86

@joelanford joelanford Awaiting requested review from joelanford

Assignees

@Adirio Adirio

@estroz estroz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@justinsb @k8s-ci-robot @Adirio @camilamacedo86 @estroz